Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Web surfer creating incomplete copy of messages #4050

Merged
merged 5 commits into from
Nov 25, 2024

Conversation

Hedrekao
Copy link

@Hedrekao Hedrekao commented Nov 4, 2024

Why are these changes needed?

Previously, generate_surfer_reply would create not a full copy of messages, it would not include the last one. This is behaviour would result in openai.BadRequestError exception, if the last message was a tool message.

image

The openAI client expects that for each assistant message with tool_call, a tool message with corresponding tool_call_id has to exist in passed messages. In the edge case where tool call response was the last message in history, web surfer agent was excluding it, which lead to showcased exception.

Once the slice operates on entire list, the issue disappears.

Related issue number

N/A

Checks

@ekzhu ekzhu requested a review from afourney November 4, 2024 15:20
@ekzhu ekzhu added the 0.2 Issues which are related to the pre 0.4 codebase label Nov 4, 2024
@ekzhu
Copy link
Collaborator

ekzhu commented Nov 12, 2024

@afourney Do you remember why we don't include the last message?

@Hedrekao
Copy link
Author

Hi guys, any chance on looking at this fix in the near future. I am aware that there might be a deeper reasoning why the last message was not included, but interaction with tools calls is causing this really nasty edge case bug.

I know that you are very busy, especially that 0.2 is not the main version that is being worked on, but it would be really awesome to fix this issue one way or another, would love to switch from using fork at work to just this project 😄

@ekzhu
Copy link
Collaborator

ekzhu commented Nov 24, 2024

@Hedrekao thanks for the reminder. I think we can accept the change if it can address the failure case without changing the behavior otherwise. For example, can you make sure to detect that if the last message is indeed a tool call message, then make sure the tool call response is included.

Thank you!

@afourney could you comment on this suggestion?

@Hedrekao
Copy link
Author

@ekzhu Sure thing, just updated the PR with only copying last message if it's in fact a tool message

@ekzhu ekzhu merged commit 758edb7 into microsoft:0.2 Nov 25, 2024
35 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.2 Issues which are related to the pre 0.4 codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants